interfaces/home: add 'read' attribute to allow non-owner read to @{HOME} #5016
Conversation
If unspecified, 'read' defaults to 'read: owner'. When 'read: all' is specified, deny connection and auto-connection (thus requiring a manual review). References: https://forum.snapcraft.io/t/home-access-as-root-from-confined-snaps/3802
|
@niemeyer: FYI, I did |
interfaces/builtin/home.go
Outdated
| // It's fine if 'read' isn't specified, but if it is, it needs to be | ||
| // either 'all' or 'owner' | ||
| if r, ok := plug.Attrs["read"]; ok { | ||
| _, ok = r.(string) |
There was a problem hiding this comment.
Perhaps this is correct but it reads as a bit off.
r is an interface (probably interface{}) so you need a type assertion to see that it is a string.
But after the test r is still interface{} because you didn't assign the type-safe r anywhere (in pseudo-code: rs, ok := r.(string). I'm not sure what go does in the case that ok is true (so r is a string) but the comparisons use an interface on the lvalue and a string literal on the rvalue.
There was a problem hiding this comment.
AFAIK Go does the right thing (although it does look 'different' from the usual pattern). Interfaces are comparable and equal if both have the same dynamic type and dynamic values are equal.
There was a problem hiding this comment.
The logic is safe, but there's another gotcha here in terms of the actual intention behind the logic: this check ignores incorrect values that have the wrong type.
We can both fix that and make the logic simpler by folding both conditionals into:
if r, ok := plug.Attrs["read"]; ok && r != "all" {
... error ...
}
There was a problem hiding this comment.
Sorry, my point about the mishandling of the wrong type is incorrect. The simplification point remains.
|
|
||
| func (iface *homeInterface) AppArmorConnectedPlug(spec *apparmor.Specification, plug *interfaces.ConnectedPlug, slot *interfaces.ConnectedSlot) error { | ||
| var read string | ||
| _ = plug.Attr("read", &read) |
There was a problem hiding this comment.
Could we use plug.Attr in the BeforePreparePlug above?
There was a problem hiding this comment.
I'm doing it this way in BeforePreparePlug because I want to see if it exists at all (since it is ok if it doesn't) and if it does make sure it is a string. I could just ignore if it isn't a string, but this isn't what we do elsewhere (eg browser-support, content and docker-support). The reason I can use plug.Attr() in AppArmorConnectedPlug is because by the time it is run, I'm sure things are ok and if read isn't an attribute, then the 'read' variable is simply empty and won't match read != "all". As such, I prefer to leave the logic as is. If people feel strongly about changing it, we should change it in all of the interfaces that do similar checks.
| func init() { | ||
| registerIface(&commonInterface{ | ||
| registerIface(&homeInterface{commonInterface{ |
There was a problem hiding this comment.
I must say I'm really happy we're doing this. It makes to-and-from common vs custom interface transitions so much easier than before.
| @@ -63,14 +76,56 @@ owner /run/user/[0-9]*/gvfs/{,**} r, | |||
| owner /run/user/[0-9]*/gvfs/*/** w, | |||
| ` | |||
|
|
|||
| const homeConnectedPlugAppArmorWithAllRead = ` | |||
| # Allow non-owner read to non-hidden and non-snap files and directories | |||
There was a problem hiding this comment.
Is this documented anywhere public? The home interface is probably the most used interface we have.
There was a problem hiding this comment.
Not yet-- this is new :) I will update the documentation for it, yes.
Codecov Report
@@ Coverage Diff @@
## master #5016 +/- ##
==========================================
+ Coverage 78.92% 78.94% +0.01%
==========================================
Files 494 494
Lines 37272 37283 +11
==========================================
+ Hits 29417 29432 +15
+ Misses 5483 5480 -3
+ Partials 2372 2371 -1
Continue to review full report at Codecov.
|
|
@niemeyer - can you respond to my question: "I did read: [all|owner] instead of read: [all|own]. If you prefer own over owner, let me know and I'll adjust". |
interfaces/builtin/home.go
Outdated
| // It's fine if 'read' isn't specified, but if it is, it needs to be | ||
| // either 'all' or 'owner' | ||
| if r, ok := plug.Attrs["read"]; ok { | ||
| _, ok = r.(string) |
There was a problem hiding this comment.
The logic is safe, but there's another gotcha here in terms of the actual intention behind the logic: this check ignores incorrect values that have the wrong type.
We can both fix that and make the logic simpler by folding both conditionals into:
if r, ok := plug.Attrs["read"]; ok && r != "all" {
... error ...
}
|
All comments are addressed in what I just committed today. |
|
Since there are two approvals and I addressed @niemeyer comments, merging. |
If unspecified, 'read' defaults to 'read: owner'. When 'read: all' is
specified, deny connection and auto-connection (thus requiring a manual
review).
References:
https://forum.snapcraft.io/t/home-access-as-root-from-confined-snaps/3802